Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix panic on debug build #8736

Closed
wants to merge 1 commit into from
Closed

fix panic on debug build #8736

wants to merge 1 commit into from
+6 −1

Conversation

ikozhukhov
Copy link
Contributor

i have panic on DEBUG build on DilOS with test send_mixed_raw

panic[cpu3]/thread=fffffe0bf62f6c40:
assertion failed: (!copied) is equivalent to (hdr->b_l1hdr.b_freeze_cksum == NULL), file: ../../common/fs/zfs/arc.c, line: 1839


fffffe0012014700 genunix:process_type+1946ed ()
fffffe0012014730 zfs:arc_buf_try_copy_decompressed_data+b7 ()
fffffe00120147c0 zfs:arc_buf_fill+2e3 ()
fffffe0012014810 zfs:arc_untransform+25 ()
fffffe00120148a0 zfs:dbuf_read+4a1 ()
fffffe0012014910 zfs:dmu_buf_hold+80 ()
fffffe00120149f0 zfs:zap_lockdir+5e ()
fffffe0012014aa0 zfs:zap_lookup_norm+6c ()
fffffe0012014b00 zfs:zap_lookup+36 ()
fffffe0012014b60 zfs:zfs_get_zplprop+fc ()
fffffe0012014bc0 zfs:nvl_add_zplprop+36 ()
fffffe0012014c10 zfs:zfs_ioc_objset_zplprops+f5 ()
fffffe0012014cc0 zfs:zfsdev_ioctl+524 ()
fffffe0012014d00 genunix:cdev_ioctl+25 ()
fffffe0012014d50 specfs:spec_ioctl+4d ()
fffffe0012014de0 genunix:fop_ioctl+55 ()
fffffe0012014ef0 genunix:ioctl+a4 ()
fffffe0012014f00 unix:brand_sys_syscall+320 ()

Motivation and Context

Description

based on research and comments, we should to check b_freeze_cksum in loop

How Has This Been Tested?

test send_mixed_raw PASS after changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ikozhukhov
fix panic on debug build
919a496 
Signed-off-by: Igor Kozhukhov <[email protected]>
@codecov
Copy link

codecov bot commented May 11, 2019 •
edited
Loading

Codecov Report

Merging #8736 into master will decrease coverage by 0.05%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8736      +/-   ##
==========================================
- Coverage   78.97%   78.92%   -0.06%     
==========================================
  Files         381      381              
  Lines      117797   117802       +5     
==========================================
- Hits        93034    92978      -56     
- Misses      24763    24824      +61
Flag Coverage Δ
#kernel 79.4% <25%> (-0.02%) ⬇️
#user 67.9% <50%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f378f42...919a496. Read the comment docs.

@ahrens ahrens added the Status: Code Review Needed Ready for review and testing label May 14, 2019
@behlendorf behlendorf requested a review from grwilson May 23, 2019 22:43
@behlendorf behlendorf mentioned this pull request Jun 10, 2019
Closed
behlendorf
behlendorf requested changes Jun 13, 2019
View reviewed changes
module/zfs/arc.c
}

/*
* There were no decompressed bufs, so there should not be a
* checksum on the hdr either.
*/
EQUIV(!copied, hdr->b_l1hdr.b_freeze_cksum == NULL);
if (cnt > 0)
EQUIV(!copied, b_freeze_cksum == NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikozhukhov given that b_l1hdr.b_freeze_cksum can only be set when ZFS_DEBUG_MODIFY is enabled (see arc_cksum_compute(). I think this EQUIV is only valid when ZFS_DEBUG_MODIFY is set, and we should instead wrap it with a conditional that checks for the flag. For the same reason, the ASSERT after the call to arc_buf_try_copy_decompressed_data() in arc_buf_fill() looks wrong to me as well.

My suspicion is we've never seen this in testing because by default for debug builds we enable ZFS_DEBUG_MODIFY. It would be great to get @grwilson's thoughts on this, since I may be misunderstanding the intent of b_freeze_cksum here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, b_freeze_cksum should only have data when ZFS_DEBUG_MODIFY is enabled. So if I understand the original problem then wrapping the EQUIV with if (zfs_flags & ZFS_DEBUG_MODIFY) would fix the problem.

@behlendorf behlendorf mentioned this pull request Jul 1, 2019
Merged
12 tasks
@behlendorf behlendorf closed this Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf requested changes

@grwilson grwilson Awaiting requested review from grwilson

Assignees
No one assigned
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ikozhukhov @behlendorf @grwilson @ahrens